Skip to content

Conversation

@hiddenmemory
Copy link

Previously, if a column had no match, it would short circuit returning None, stopping subsequent columns from potentially matching.

This change means that non-matching columns are scored as zero, allowing subsequent columns to be matched against.

If the match has zero score after all columns, then we return None, retaining previous behaviour.

All existing tests pass.

Previously, if a column had no match, it would short circuit
returning None.

This means that if you have a match term on a
second column, but the first didn't match, you wouldn't get any
match for the row.

This change means that non-matching columns are scored as zero,
and then if the match has zero score after all columns, then we
return zero.
@pascalkuthe
Copy link
Member

this is not how columns are intended to be used. It's an AND match not an OR match. For example in helix diagnostic picker %severity ERR %message type will show you all type errors. With this change that would not work anymore.

@alexrutar
Copy link
Contributor

Would it be worth permitting more general (user-provided) column score strategies? Something like making the MultiPattern::score function generic over a "score strategy" trait.

This would handle the comment in the codebase about potentially allowing column weights.

Then, in principle, as long as the "score strategy" trait is sufficiently general, someone could implement the behaviour suggested in this PR if desired.

@pascalkuthe
Copy link
Member

I would be more option to that but I think the better way to do this would be to add general OR predicate (the one aspect of FZF we haven't ported yet) and then we could have a general mechanism to include OR patterns that would also apply to columns (like colum1: foo & column2: bar | colum3: baz).

@alexrutar alexrutar mentioned this pull request Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants